Use npm instead of yarn to build Theia #14481

tsmaeder · 2024-11-19T13:23:49Z

What it does

This PR rewrites the Theia build process to use npm in stead of yarn. This required a couple of changes:

Contrary to yarn, npm does not invoke the prepare scripts of workspace packages in the order of their dependencies (npm/cli#3034). So instead of relying on yarn install to invoke the necessary build scripts, we have to invoke the build process explicitly after the npm install. The build process is built on this logic:

Each package has a script called "build". The goal of that target is to fully build all artifacts this repo produces. For regular Theia packages, this is just incrementally compiling the typescript sources of the package an it's dependencies, but for applications, for example, it means creating the application bundle. So calling "build" in an application package should be all that's needed in regular development
To build the repo initially after check-out, there are a couple of additinal steps necesary:
- compile build tools (re-exports package)
- generate re-exports where applicable
- compile all packages
- build alll application packages
  This can be achieved by invoking npm run build in the repo root.

What's missing:

Doc is not up to date
linting is currently broken

Fixes #13948

Contributed on behalf of STMicroelectronics

How to test

Make sure we can build Theia as described above and the apps work fine.

Follow-ups

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

Fixes eclipse-theia#13948 Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>

Signed-off-by: Thomas Mäder <[email protected]>

due to eclipse-dash/dash-licenses#415 Signed-off-by: Thomas Mäder <[email protected]>

dankeboy36 · 2024-11-24T17:32:41Z

Fixes #13948

Possible related: #775

Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder · 2024-11-25T15:44:03Z

@msujew @sdirix the PR checks now pass and it's ready to be reviewed minus doc.

sdirix

So far it works very well.

I used npm ci to install. I tested all applications and was able to build and start all of them. I also tested watching for the browser application. Tests also completed successfully.

Linting is currently broken but this is already mentioned in the current PR description.

@theia/request: > @theia/[email protected] lint
@theia/request: > theiaext lint
@theia/request: $ eslint --cache=true --no-error-on-unmatched-pattern=true "{src,test}/**/*.{ts,tsx}"
@theia/request: /bin/sh: 1: node: not found
@theia/request: Oops! Something went wrong! :(
@theia/request: ESLint: 8.57.1
@theia/request: SyntaxError: Failed to load plugin '@theia' declared in '.eslintrc.js » ../../configs/build.eslintrc.json » ./base.eslintrc.json': Unexpected end of JSON input
@theia/request:     at JSON.parse (<anonymous>)
@theia/request:     at PackageReExports.FromPackageSync (/home/user/git/theia/dev-packages/private-re-exports/lib/package-re-exports.js:127:47)
@theia/request:     at Object.<anonymous> (/home/user/git/theia/dev-packages/private-eslint-plugin/rules/shared-dependencies.js:24:40)

sdirix · 2024-11-27T20:14:45Z

.editorconfig

@@ -1,7 +1,6 @@
 root = true

 [*]
-insert_final_newline = true


Can we keep this? Why remove it?

sdirix · 2024-11-27T20:15:47Z

.github/workflows/ci-cd.yml

-          yarn global add node-gyp
-          yarn --skip-integrity-check --network-timeout 100000
+          npm install -g node-gyp
+          npm install


I think we should use npm ci instead of npm install in all workflows to have a more deterministic pipeline. Using ci will not update any dependencies and install them exactly as they are contained in the package-lock.json

sdirix · 2024-11-27T20:18:36Z

dev-packages/application-manager/src/generator/webpack-generator.ts

+                await this.write(this.genConfigPath, this.compileWebpackConfig());
        if (!this.pck.isBrowserOnly()) {
            await this.write(this.genNodeConfigPath, this.compileNodeWebpackConfig());
        }
        if (await this.shouldGenerateUserWebpackConfig()) {
            await this.write(this.configPath, this.compileUserWebpackConfig());
        }
-    }
+            }


Seems like accidental changes

sdirix · 2024-11-27T20:20:14Z

dev-packages/private-ext-scripts/package.json

@@ -23,4 +22,4 @@
    "ext:test:watch": "mocha -w --config ../../configs/mocharc.yml \"./lib/**/*.*spec.js\"",
    "ext:test:clean": "rimraf .nyc_output coverage"
  }
-}
+}


We should keep the newline. I guess this change comes in via the changed .editorconfig.

sdirix · 2024-11-27T20:22:58Z

package.json

@@ -117,4 +115,4 @@
    "ms-vscode.references-view",
    "ms-python.vscode-pylance"
  ]
-}
+}


same here with the newline

sdirix · 2024-11-27T20:39:17Z

package.json

    "preinstall": "node-gyp install",
-    "postinstall": "theia-patch",
-    "prepare": "yarn -s compile:references && lerna run prepare && yarn -s compile",
+    "postinstall": "theia-patch && npm run -s compute-references && lerna run afterInstall",


Personal preference

I would add the compile step here. I don't think there are many use cases where someone wants to run install but not also compile all Typescript files.

Suggested change

"postinstall": "theia-patch && npm run -s compute-references && lerna run afterInstall",

"postinstall": "theia-patch && npm run -s compute-references && lerna run afterInstall && npm run compile",

sdirix · 2024-11-27T20:46:41Z

package.json

-    "performance:startup:electron": "yarn -s electron rebuild && cd scripts/performance && node electron-performance.js --name 'Electron Frontend Startup' --folder electron --runs 10",
+    "performance:startup": "npm run -s performance:startup:browser && npm run -s performance:startup:electron",
+    "performance:startup:browser": "concurrently --success first -k -r \"cd scripts/performance && node browser-performance.js --name 'Browser Frontend Startup' --folder browser --runs 10\" \"npm run -s --cwd examples/browser start\"",
+    "performance:startup:electron": "npm run -s electron rebuild && cd scripts/performance && node electron-performance.js --name 'Electron Frontend Startup' --folder electron --runs 10",


It would be nice to also have the watch scripts here, e.g. watch, watch:browser etc.

sdirix · 2024-11-27T20:50:06Z

package.json

    "publish:check": "node scripts/check-publish.js",
    "rebuild:clean": "rimraf .browser_modules",
-    "test": "yarn -s test:theia && yarn -s electron test && yarn -s browser test",
+    "rebuild:browser": "cd examples/browser && npm run rebuild",


If we offer rebuild:browser we should also offer rebuild:electron

sdirix · 2024-11-27T20:53:10Z

dev-packages/private-ext-scripts/package.json

-    "ts-clean": "theia-ts-clean.js"
+    "run": "bin/theia-run.js",
+    "theiaext": "bin/theia-ext.js",
+    "ts-clean-dangling": "bin/theia-ts-clean.js"


I like this rename 👍

sdirix · 2024-11-27T20:54:34Z

package.json

    "publish:check": "node scripts/check-publish.js",
    "rebuild:clean": "rimraf .browser_modules",
-    "test": "yarn -s test:theia && yarn -s electron test && yarn -s browser test",
+    "rebuild:browser": "cd examples/browser && npm run rebuild",
+    "start:browser": "cd examples/browser && npm run start",


start:browser-only is missing

tsmaeder requested review from msujew and sdirix November 19, 2024 13:23

tsmaeder force-pushed the npm_experiment branch from 79e2f1c to 43cf4ef Compare November 22, 2024 09:06

tsmaeder added 17 commits November 24, 2024 13:05

Use npm instead of yarn to build Theia

c5b0db8

Fixes eclipse-theia#13948 Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>

Update workflows, etc. to use npm

c64dfcb

Signed-off-by: Thomas Mäder <[email protected]>

Fix ci build

4ac4643

Signed-off-by: Thomas Mäder <[email protected]>

Show git status

a5080ac

Signed-off-by: Thomas Mäder <[email protected]>

Add .gitattributes to prevent crlf conversion at install

312bd04

Signed-off-by: Thomas Mäder <[email protected]>

Further fixes for crlf handling.

abbaf15

Signed-off-by: Thomas Mäder <[email protected]>

Fix theia-ext.js script

ef7d5e7

Signed-off-by: Thomas Mäder <[email protected]>

chmod + x for bin files

e2dd83b

Signed-off-by: Thomas Mäder <[email protected]>

Fix more exec flags

d4e23a9

Signed-off-by: Thomas Mäder <[email protected]>

Fix passing script parameters

51bacae

Signed-off-by: Thomas Mäder <[email protected]>

Undo markdown-it update

7e91fb2

Signed-off-by: Thomas Mäder <[email protected]>

Updated readme.

977c252

Signed-off-by: Thomas Mäder <[email protected]>

Update packag-lock.json

8112326

Signed-off-by: Thomas Mäder <[email protected]>

Adapt to change in mocha internals we use.

e37f2b5

Signed-off-by: Thomas Mäder <[email protected]>

More PR build fixes

1c22140

Signed-off-by: Thomas Mäder <[email protected]>

Use package-lock.json in license check

5336f62

Signed-off-by: Thomas Mäder <[email protected]>

Add plugin packages to ip-baseline

82b7da0

due to eclipse-dash/dash-licenses#415 Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder force-pushed the npm_experiment branch from 70c2767 to 82b7da0 Compare November 24, 2024 12:07

dankeboy36 mentioned this pull request Nov 24, 2024

consider moving from yarn to npm arduino/vscode-arduino-tools#20

Open

tsmaeder added 7 commits November 25, 2024 10:00

Downgrade eslint and add used plugins to config

3e668e8

Signed-off-by: Thomas Mäder <[email protected]>

Fix lerna invocation

dd67398

Signed-off-by: Thomas Mäder <[email protected]>

Use compile instead of build to create linter plugin

f9589e7

Signed-off-by: Thomas Mäder <[email protected]>

Fix build before lint

bdb6fb6

Signed-off-by: Thomas Mäder <[email protected]>

Refactor postinstall scripts

469782c

Signed-off-by: Thomas Mäder <[email protected]>

Fix playwright tests

6163af0

Signed-off-by: Thomas Mäder <[email protected]>

Fix playwright ci script

652efeb

Signed-off-by: Thomas Mäder <[email protected]>

Fix production smoke test

37d8330

Signed-off-by: Thomas Mäder <[email protected]>

sdirix reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use npm instead of yarn to build Theia #14481

Use npm instead of yarn to build Theia #14481

tsmaeder commented Nov 19, 2024

dankeboy36 commented Nov 24, 2024

tsmaeder commented Nov 25, 2024

sdirix left a comment •

edited

Loading

sdirix Nov 27, 2024

sdirix Nov 27, 2024

sdirix Nov 27, 2024

sdirix Nov 27, 2024

sdirix Nov 27, 2024

sdirix Nov 27, 2024

sdirix Nov 27, 2024

sdirix Nov 27, 2024

sdirix Nov 27, 2024

sdirix Nov 27, 2024

	"postinstall": "theia-patch && npm run -s compute-references && lerna run afterInstall",
	"postinstall": "theia-patch && npm run -s compute-references && lerna run afterInstall && npm run compile",

Use npm instead of yarn to build Theia #14481

Are you sure you want to change the base?

Use npm instead of yarn to build Theia #14481

Conversation

tsmaeder commented Nov 19, 2024

What it does

How to test

Follow-ups

Review checklist

Reminder for reviewers

dankeboy36 commented Nov 24, 2024

tsmaeder commented Nov 25, 2024

sdirix left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Personal preference

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdirix left a comment •

edited

Loading